-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor credentials from error handling #94682
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
import { useSiteMigrationCredentialsMutation } from './use-site-migration-credentials-mutation'; | ||
|
||
const mapApiError = ( error: any ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this response transformation to reduce complexity
recordTracksEvent( 'calypso_site_migration_automated_request_error' ); | ||
}, | ||
} ); | ||
const serverSideError = useFormErrorMapping( error, variables ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All error logic is now parsed by this hook and returned on the serverSideError variable
clearErrors, | ||
} = useForm< CredentialsFormData >( { | ||
mode: 'onSubmit', | ||
reValidateMode: 'onSubmit', | ||
disabled: isPending, | ||
defaultValues: { | ||
siteAddress: importSiteQueryParam, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backend is waiting for a from_url,
and the front is calling as siteAddress.
It was causing constantly process of map values to show errors. I just removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Fixed some merge conflicts and some test failing on and off. Merging to unblock another ticket
<Controller | ||
control={ control } | ||
name="siteAddress" | ||
name="from_url" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are switching to snake case here, but backupFileLocation
is in camel case. Having two different cases for the form fields in the same form looks a bit confusing to me. WDYT about sticking to one of the cases for all form fields instead?
Related to #
Proposed Changes
Testing Instructions
Base:
Scenario 1: (Credentials - Required Fields)
Scenario 2: (Credentials - Create)
Scenario 3: (Backup file - Required Fields)
Scenario 4: (Backup - Create)
NOTE: These and more scenarios are tested via this test suite
Pre-merge Checklist